feat(cli): allow generic file uploads including zip - #4880
Conversation
Agent co-lab was blocked on skill packs because buzz-cli only allowed image/* and video/mp4. The relay already accepts generic Blossom uploads via a danger denylist (HTML/JS/SVG/executables). Align the CLI with that policy so zip/pdf/text and similar types work while unsafe types stay blocked. Add unit coverage for allow/block and size tiers (100MB generic). Signed-off-by: Trevor P <trev2005@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1562cc9cda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if mime.starts_with("audio/") { | ||
| return false; | ||
| } | ||
| !BLOCKED_FILE_MIMES.contains(&mime) |
There was a problem hiding this comment.
Render generic attachments as links
Allowing every non-blocklisted MIME here also enables buzz messages send --file <pdf|zip|txt>, but that send path still formats every uploaded non-video descriptor as  in crates/buzz-cli/src/commands/messages.rs:622-626. Desktop's generic file renderer keys on a plain [filename](url) link with matching non-media imeta, so these newly allowed attachments render as broken inline images instead of download cards; update the CLI message formatting before broadening this allowlist.
Useful? React with 👍 / 👎.
|
Independent prod verification, in case it's useful for review — I hit the exact One thing I liked reviewing the diff: `is_upload_mime_allowed()` mirrors the relay's denylist rather than hand-rolling a parallel allowlist, so the CLI won't silently drift out of sync the next time `buzz-media` adds a MIME. That's a nicer factoring than a static list would have been. |
Codex P2 on block#4880: after widening upload MIME types, `messages send --file` still emitted `` for every non-video blob. Desktop FileCard expects plain `[filename](url)` plus imeta `filename` for zip/pdf/txt. Match Desktop formatImetaMediaLine: images/video stay inline; generic files and agent/team snapshot PNGs use markdown links; escape label metacharacters; include basename on imeta tags. Signed-off-by: Trevor P <trev2005@gmail.com>
Codex P2 on block#4880: after widening upload MIME types, `messages send --file` still emitted `` for every non-video blob. Desktop FileCard expects plain `[filename](url)` plus imeta `filename` for zip/pdf/txt. Match Desktop formatImetaMediaLine: images/video stay inline; generic files and agent/team snapshot PNGs use markdown links; escape label metacharacters; include basename on imeta tags. Signed-off-by: Trevor P <trev2005@gmail.com>
1d36957 to
5fb4c1a
Compare
|
Addressed the Codex P2 (generic attachments as links):
Pushed on this PR as |
After routing `messages send --file` through `build_imeta_tag_with_filename`, the thin `build_imeta_tag` wrapper had no non-test caller, so the plain lib build tripped `-D warnings` dead_code and failed the Rust Lint CI gate. Fold its doc comment into `build_imeta_tag_with_filename` and have the test call the `(&d, None)` form directly. Signed-off-by: Ravneet Arora <rarora@squareup.com> Co-authored-by: Cursor <cursoragent@cursor.com>
wesbillman
left a comment
There was a problem hiding this comment.
Review submitted by Carl on Wes's behalf.
The upload policy and generic-link formatting are sound, but the new filename field needs to obey the relay validation it targets before this can merge.
[P2] Validate or omit relay-invalid basenames before publishing — build_imeta_tag_with_filename currently appends every non-empty basename unchanged. On Unix, filenames may contain \\ and control characters (including newlines); the relay explicitly rejects either in crates/buzz-relay/src/handlers/imeta.rs (imeta filename must not contain path separators or control characters). cmd_send_message now adds this field for every attachment, so a previously sendable image such as report\\final.png uploads successfully and then the message publish fails, leaving an orphaned blob. Please apply the relay's 1–255-byte / no-separator / no-control validation before upload or omit the filename field when invalid, and cover the invalid-basename case. The markdown label should follow the same safe fallback if the field is omitted.
wesbillman
left a comment
There was a problem hiding this comment.
One blocking regression: the new filename field is emitted without applying the relay's filename constraints, so valid local files can upload successfully but then make messages send --file fail at event ingest. Please sanitize the basename using the same contract as Desktop (strip either path separator, remove controls, bound to 255 bytes/characters as required, and provide a fallback), and use that same sanitized label for both imeta and markdown. The current Windows CI failure is in unrelated buzz-dev-mcp shell timeout tests, not this diff.
| .file_name() | ||
| .and_then(|s| s.to_str()) | ||
| .unwrap_or(name); | ||
| tag.push(format!("filename {base}")); |
There was a problem hiding this comment.
Blocking: This does not actually guarantee a relay-valid filename. On Unix, Path::file_name() leaves bad\\name.zip unchanged; control characters are also retained, and names longer than 255 are unbounded. The relay rejects all three (crates/buzz-relay/src/handlers/imeta.rs:140-155), while Desktop explicitly strips both separator styles, controls, and caps the label (desktop/src-tauri/src/commands/media.rs:138-153). Because cmd_send_message now attaches filename to every upload, even an otherwise valid image with such a local name uploads first and then fails message ingest—a regression from the old filename-free tag. Please centralize a sanitizer matching the relay contract and use its result for both this field and the markdown label. Add cases for backslashes, controls, and oversized/multibyte names.
Duplicate of the later changes-requested review; both identified the same filename-validation blocker. Keeping the more precise review as the single actionable request.
Wes requested changes on block#4880: Path::file_name alone left backslashes, controls, and oversize names intact, so Blossom upload could succeed while messages send --file failed at event ingest. Add sanitize_filename (strip / and \, drop controls, bound 255 UTF-8 bytes, fallback "file") and use it for both imeta filename and markdown labels. Signed-off-by: Trevor P <trev2005@gmail.com>
|
Thanks for the careful review, Wes (and Carl). Agree on the blocking point: we should sanitize basenames to the same contract as Desktop / relay Addressed in
Windows CI timeout remains out of scope for this diff (as you noted). Ready for another look when you have a moment — re-requesting review. |
Summary
buzz-cliupload MIME policy so agent co-lab can attach zip skill packs and other safe generic files (pdf, text, json, common archives/docs).buzz-media(HTML/JS/SVG, native executables/installers); images andvideo/mp4unchanged.BUZZ_MAX_FILE_BYTES).The relay already routes non-image uploads through
process_file_upload+ denylist. The CLI was the choke (unsupported file type: application/zip).Dogfood
Test plan
cargo test -p buzz-cli --lib upload_buzz upload file --file pack.zipagainst a local or private relay